fix(security): replace wildcard CORS with allowlist and harden session cookies#467
Open
Ridanshi wants to merge 2 commits into
Open
fix(security): replace wildcard CORS with allowlist and harden session cookies#467Ridanshi wants to merge 2 commits into
Ridanshi wants to merge 2 commits into
Conversation
…n cookies
- Replace `cors('*')` with a function-based origin allowlist driven by the
FRONTEND_ORIGIN environment variable, so ACAO headers are only set for the
configured origin and are absent for all other origins.
- Add `credentials: true` to the CORS config so the browser can send the
session cookie on cross-origin requests.
- Set `httpOnly: true`, `secure` (production-only), and `sameSite: 'strict'`
on the session cookie to prevent XSS exfiltration, plaintext transmission,
and CSRF via cross-site navigations.
- Add `withCredentials: true` to the Axios calls in Login and Signup so the
browser includes the session cookie on cross-origin requests.
- Extract startup environment validation into `backend/config/validateEnv.js`
so the server refuses to start in production when FRONTEND_ORIGIN is unset,
while falling back to localhost:5173 in development with a logged warning.
Fixes GitMetricsLab#454
…alidation - Rewrite auth.routes.spec.cjs with a shared outer describe for MongoDB- dependent tests to eliminate connection conflicts under Jasmine's random seed ordering. - Resolve mongoose and passport from the backend directory in both spec files so tests and application code share one module instance, preventing operation-buffering timeouts when backend/node_modules is present. - Add CORS behaviour suite: allowed origin, disallowed origin, preflight, and credentialed-login assertions. - Add session cookie flags suite: HttpOnly and SameSite=Strict assertions. - Add environment validation suite: production throws without FRONTEND_ORIGIN, development does not. - Update test passwords to satisfy the signupSchema regex so signup and logout route tests exercise the full validation path. - Add jasmine and supertest as devDependencies in backend/package.json. - Add cors as a devDependency in root package.json for test app setup. - Add .env.example and backend/.env.example documenting all required variables including FRONTEND_ORIGIN. - Add Environment Variables table to README setup guide.
✅ Deploy Preview for github-spy ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
📝 WalkthroughWalkthroughBackend CORS configuration transitions from permissive wildcard to explicit frontend origin allowlist, session cookies gain security flags (httpOnly, secure, sameSite), environment variables are validated at startup, frontend auth requests include credentials, and comprehensive test coverage validates CORS behavior, cookie security, and configuration enforcement. ChangesCORS Allowlist & Session Cookie Security
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🎉 Thank you @Ridanshi for your contribution. Please make sure your PR follows https://github.com/GitMetricsLab/github_tracker/blob/main/CONTRIBUTING.md#-pull-request-guidelines
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
spec/auth.routes.spec.cjs (1)
74-85:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRestore
FRONTEND_ORIGINafter this suite.This suite mutates global process env in setup but does not restore it in teardown, which can leak state into other specs and cause order-dependent failures.
Suggested patch
describe('Backend auth integration', () => { let app; + let hadFrontendOrigin; + let savedFrontendOrigin; beforeAll(async () => { + hadFrontendOrigin = Object.prototype.hasOwnProperty.call(process.env, 'FRONTEND_ORIGIN'); + savedFrontendOrigin = process.env.FRONTEND_ORIGIN; process.env.FRONTEND_ORIGIN = ALLOWED_ORIGIN; await mongoose.connect('mongodb://127.0.0.1:27017/github_tracker_test'); app = createTestApp(); }); afterAll(async () => { + if (hadFrontendOrigin) { + process.env.FRONTEND_ORIGIN = savedFrontendOrigin; + } else { + delete process.env.FRONTEND_ORIGIN; + } if (mongoose.connection.readyState === 1) { await mongoose.connection.db.dropDatabase(); await mongoose.disconnect(); } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spec/auth.routes.spec.cjs` around lines 74 - 85, The test suite sets process.env.FRONTEND_ORIGIN in beforeAll but never restores it; capture the original value at start of beforeAll (e.g., const _origFrontendOrigin = process.env.FRONTEND_ORIGIN) and in afterAll restore it (process.env.FRONTEND_ORIGIN = _origFrontendOrigin or delete process.env.FRONTEND_ORIGIN if originally undefined) so that the global environment is returned to its prior state; update the beforeAll/afterAll blocks (referencing beforeAll, afterAll, process.env.FRONTEND_ORIGIN, and ALLOWED_ORIGIN) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/config/validateEnv.js`:
- Around line 6-13: The validateEnv function only enforces FRONTEND_ORIGIN in
production but must also ensure a strong session secret; update validateEnv() to
check process.env.SESSION_SECRET when NODE_ENV === 'production' and throw a
descriptive Error if it's missing or clearly insecure (e.g., empty, placeholder
like "changeme", or shorter than a minimum length such as 32 chars). Reference
the validateEnv function and add the SESSION_SECRET checks and error message so
production cannot start without a valid SESSION_SECRET.
In `@backend/server.js`:
- Around line 59-63: The cookie config sets secure: process.env.NODE_ENV ===
'production' but Express is not configured to trust a TLS-terminating proxy, so
secure cookies will not be sent; add app.set('trust proxy', true) (or a more
specific trust value) in backend/server.js during app initialization (before
middleware that sets cookies/session) so Express will honor the secure flag
behind proxies; update any related comments and ensure this runs only in
environments where a proxy is present (e.g., conditionally when NODE_ENV ===
'production' or when an env var like TRUST_PROXY is set).
---
Outside diff comments:
In `@spec/auth.routes.spec.cjs`:
- Around line 74-85: The test suite sets process.env.FRONTEND_ORIGIN in
beforeAll but never restores it; capture the original value at start of
beforeAll (e.g., const _origFrontendOrigin = process.env.FRONTEND_ORIGIN) and in
afterAll restore it (process.env.FRONTEND_ORIGIN = _origFrontendOrigin or delete
process.env.FRONTEND_ORIGIN if originally undefined) so that the global
environment is returned to its prior state; update the beforeAll/afterAll blocks
(referencing beforeAll, afterAll, process.env.FRONTEND_ORIGIN, and
ALLOWED_ORIGIN) accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6708409e-4e7e-4b6b-a7b5-4f6a243abf95
📒 Files selected for processing (11)
.env.exampleREADME.mdbackend/.env.examplebackend/config/validateEnv.jsbackend/package.jsonbackend/server.jspackage.jsonspec/auth.routes.spec.cjsspec/user.model.spec.cjssrc/pages/Login/Login.tsxsrc/pages/Signup/Signup.tsx
Comment on lines
+6
to
+13
| function validateEnv() { | ||
| if (process.env.NODE_ENV === 'production' && !process.env.FRONTEND_ORIGIN) { | ||
| throw new Error( | ||
| 'FRONTEND_ORIGIN environment variable is required in production. ' + | ||
| 'Set it to the URL of your frontend (e.g., https://app.example.com).' | ||
| ); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Require SESSION_SECRET validation in production.
validateEnv() currently checks only FRONTEND_ORIGIN, so production can still start with an empty or placeholder session secret, which weakens session integrity.
🔧 Suggested patch
function validateEnv() {
- if (process.env.NODE_ENV === 'production' && !process.env.FRONTEND_ORIGIN) {
+ const isProduction = process.env.NODE_ENV === 'production';
+ const frontendOrigin = process.env.FRONTEND_ORIGIN?.trim();
+ const sessionSecret = process.env.SESSION_SECRET?.trim();
+
+ if (isProduction && !frontendOrigin) {
throw new Error(
'FRONTEND_ORIGIN environment variable is required in production. ' +
'Set it to the URL of your frontend (e.g., https://app.example.com).'
);
}
+
+ if (isProduction && (!sessionSecret || sessionSecret === 'replace-with-a-long-random-string')) {
+ throw new Error(
+ 'SESSION_SECRET must be set to a strong random value in production.'
+ );
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/config/validateEnv.js` around lines 6 - 13, The validateEnv function
only enforces FRONTEND_ORIGIN in production but must also ensure a strong
session secret; update validateEnv() to check process.env.SESSION_SECRET when
NODE_ENV === 'production' and throw a descriptive Error if it's missing or
clearly insecure (e.g., empty, placeholder like "changeme", or shorter than a
minimum length such as 32 chars). Reference the validateEnv function and add the
SESSION_SECRET checks and error message so production cannot start without a
valid SESSION_SECRET.
Comment on lines
+59
to
+63
| cookie: { | ||
| httpOnly: true, | ||
| // Only transmit the cookie over HTTPS in production. | ||
| secure: process.env.NODE_ENV === 'production', | ||
| sameSite: 'strict', |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether secure cookies are enabled without trust proxy in backend/server.js
rg -n -C2 "secure:\s*process\.env\.NODE_ENV === 'production'|app\.set\(\s*['\"]trust proxy['\"]" backend/server.jsRepository: GitMetricsLab/github_tracker
Length of output: 245
Configure Express trust proxy for production secure cookies
backend/server.js enables secure via secure: process.env.NODE_ENV === 'production', but it doesn’t set app.set('trust proxy', ...) in this file. Behind a TLS-terminating proxy, Express must trust the proxy for secure-cookie handling.
🔧 Suggested patch
const app = express();
+
+if (process.env.NODE_ENV === 'production') {
+ app.set('trust proxy', 1);
+}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/server.js` around lines 59 - 63, The cookie config sets secure:
process.env.NODE_ENV === 'production' but Express is not configured to trust a
TLS-terminating proxy, so secure cookies will not be sent; add app.set('trust
proxy', true) (or a more specific trust value) in backend/server.js during app
initialization (before middleware that sets cookies/session) so Express will
honor the secure flag behind proxies; update any related comments and ensure
this runs only in environments where a proxy is present (e.g., conditionally
when NODE_ENV === 'production' or when an env var like TRUST_PROXY is set).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes
Closes #454
Problem
backend/server.jsappliedcors('*'), which:Access-Control-Allow-Origin: *on every response — including authenticated endpoints — making it impossible to attach session cookies to cross-origin requests (browsers refusecredentialswith a wildcard ACAO).SameSite,Secure, or explicitHttpOnlyattributes — cookie transmitted in plaintext over HTTP and vulnerable to CSRF via cross-site navigations.FRONTEND_ORIGINin production would silently degrade to an open wildcard, with no operator-visible error.Changes
backend/config/validateEnv.js(new)Extracts startup environment validation into a standalone, unit-testable module. In production the server calls
process.exit(1)with a clear error ifFRONTEND_ORIGINis unset; in development it defaults tohttp://localhost:5173and logs a warning.backend/server.jscors('*')with a function-based origin allowlist driven byFRONTEND_ORIGIN.A string value in the
corsoriginoption sends the header unconditionally; a function is required to suppress the header entirely for unlisted origins.credentials: trueso the browser sends the session cookie on cross-origin requests.httpOnly: true,secure(production-only), andsameSite: 'strict'to the session cookie.src/pages/Login/Login.tsx·src/pages/Signup/Signup.tsxwithCredentials: trueto both Axios calls so the browser attaches and stores the session cookie for cross-origin requests.spec/auth.routes.spec.cjsdescribeto avoid Mongoose connection conflicts with Jasmine's random seed ordering.mongoose,passport, andexpress-sessionfrom the backend directory so the test app and the application code share one module instance (prevents operation-buffering timeouts whenbackend/node_modulesexists).HttpOnlyandSameSite=Strictassertions.FRONTEND_ORIGIN, non-production does not.signupSchemaZod regex so signup and logout route tests exercise the full validation path.spec/user.model.spec.cjsrequire.resolvefix so the User model tests share the backend's mongoose instance.backend/.env.example·.env.example(new)Documents all required environment variables, including
FRONTEND_ORIGIN.README.mdAdds an Environment Variables table to the setup guide.
New environment variable
FRONTEND_ORIGINhttp://localhost:5173Test results
Behaviour preserved
FRONTEND_ORIGINdefaults automatically.Test plan
backend/.env.example→backend/.env; setFRONTEND_ORIGIN=http://localhost:5173; confirm server starts.HttpOnly; SameSite=Strict.Origin: http://evil.example.com; confirm noAccess-Control-Allow-Originheader in the response.FRONTEND_ORIGIN, setNODE_ENV=production; confirm the server refuses to start.npm run test:backend— all 20 tests pass.Summary by CodeRabbit
Documentation
Security
Tests
Chores